Skip to content

fix(ui): resolve interactive selections by index, not display text - #68

Open
aaearon wants to merge 8 commits into
mainfrom
test/ui-behavior
Open

fix(ui): resolve interactive selections by index, not display text#68
aaearon wants to merge 8 commits into
mainfrom
test/ui-behavior

Conversation

@aaearon

@aaearon aaearon commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Part 7 of 8. Base: test/isolation-harness (#63) — must merge first.

⚠️ Two real user-facing bugs

Both SelectGroup and SelectTarget bound survey.Select to a string and resolved the user's choice by display text.

  • Groups: two Entra ID groups with the same name in different directories render identically, so FindGroupByDisplay returned the first match regardless of which row was highlighted. Pick the second, elevate into the first — while DirectoryID, the field that exists to disambiguate them, was never consulted.
  • Targets: worse. SelectTarget rendered a sorted option list but resolved against the caller's unsorted slice, and FormatTargetOption contains no ID — just workspace type, workspace name, role name, CSP. Two targets with the same workspace name and role in different subscriptions collide.

Both now bind to selectedIdx and resolve through a bounds-checked helper, matching SelectRole and SelectRequest, which already did this correctly and carried comments saying duplicate display strings are safe.

How this was missed

A previous fix sorted a copy and left a comment claiming it "avoids wrong-group selection on display collisions". It never did — sorting only makes the wrong answer deterministic. The audit read that comment, classified the missing sort test as a wrong-group regression guard, and the plan repeated it. The test written to pin it turned out to be tautological: for any slice, a linear exact-match scan is guaranteed to find an element rendering to the string it was given, so the assertion held identically for unsorted input.

Only mutating the actual call site exposed it.

Also

  • IsInteractive() never asserted it inspects stdin. Swapping to os.Stdout.Fd() survived — that swap is what would make grant revoke < /dev/null hang forever in a terminal.
  • Guard ordering (non-interactive before empty-list) was correct but unpinned in all five selectors; swapping them survived. Now covered.
  • Boundary and format gaps: exactly-zero remaining, RFC3339Nano fractional seconds, case-insensitive role sort, and the three missing _EmptyList tests.
  • sort.Slicesort.SliceStable so colliding rows keep input order.

Notes

  • FindGroupByDisplay and FindTargetByDisplay are now production-dead — only tests call them. Kept with accurate doc comments (no production caller; returns the first match on a collision) and flagged as follow-up deletion candidates, rather than removed inside a behaviour-fix commit.
  • SelectSessions still resolves by text and is left alone: its display string embeds SessionID, so collisions are unreachable. That safety now rests on BuildSessionOptions continuing to embed the ID — asserted by comment, not by test.
  • The won't-fix bounds checks in SelectRole/SelectRequest were confirmed unreachable by reading survey/v2 source: Index always derives from the original options slice.
  • Honest caveat on the proofs: survey.Select cannot be driven from a test, so "before" means the new helper with the old display-text resolution.

Adversarial review performed (Codex credits exhausted; review by a Claude agent) — it found the group bug; the SelectTarget instance was found while fixing it.

@aaearon aaearon closed this Aug 15, 2026
@aaearon aaearon reopened this Aug 15, 2026
@aaearon aaearon closed this Aug 15, 2026
@aaearon aaearon reopened this Aug 15, 2026
@aaearon
aaearon deleted the branch main August 16, 2026 07:34
@aaearon aaearon closed this Aug 16, 2026
@aaearon aaearon reopened this Aug 16, 2026
@aaearon
aaearon changed the base branch from test/isolation-harness to main August 16, 2026 07:37
Closes the eight actionable internal/ui mutation-ledger rows (UI-01..UI-08)
and records UI-09/UI-10 as closed wont-fix.

- IsInteractive() now has a test asserting WHICH fd is probed; every previous
  stub ignored its argument, so swapping os.Stdin.Fd() for os.Stdout.Fd()
  survived — the swap that would make `grant revoke < /dev/null` hang.
- Extract sortGroupsForDisplay from SelectGroup (behaviour-preserving) so the
  display-collision ordering fix is testable without a TTY.
- Add the exactly-zero remaining-time boundary, an RFC3339Nano timestamp case,
  a mixed-case role sort fixture, and the missing empty-list guards for
  SelectTarget, SelectSessions and SelectGroup.
SelectGroup bound survey.Select to a string and recovered the group with
FindGroupByDisplay. Two groups with the same name in different directories
render identically, so the lookup returned the first match regardless of which
row the user highlighted - highlighting the second elevated into the first.
Sorting a copy never fixed that; it only made the wrong answer deterministic.

SelectGroup now binds an int and resolves through resolveGroupSelection,
matching SelectRole and SelectRequest. The sort becomes SliceStable so
colliding rows keep their input order.

Tests:
- TestResolveGroupSelection_DuplicateDisplayStrings pins the index path; it
  fails when the resolver is reverted to a display lookup.
- TestSortGroupsForDisplay_Ordering replaces the collision test, dropping its
  tautological FindGroupByDisplay round-trip and claiming only what it pins:
  display ordering plus a full-snapshot caller-slice immutability check.
- TestSelect{Target,Sessions,Group,Role,Request}_Non{TTY,Interactive}EmptyList
  pin the non-interactive guard ahead of the empty-list guard; all five fail
  when the guards are swapped.
- Role option/role parallelism and a length assertion; exact-match assertion
  in the request timestamp test.

Docs: CHANGELOG Fixed entry; mutation-ledger UI-02/06/08 line references
repointed and UI-02's narrative corrected to list ordering only.
SelectTarget rendered BuildOptions(targets) - a sorted string slice - and then
recovered the answer with FindTargetByDisplay against the caller's unsorted
slice. FormatTargetOption carries no ID, so two eligible targets with the same
workspace name and role in different subscriptions or accounts render
identically; the lookup returned the first match whatever the user highlighted,
and because the rendered and searched slices were in different orders the two
could disagree even without a collision. Wrong-target elevation either way.

SelectTarget now renders from sortTargetsForDisplay and resolves through
resolveTargetSelection by index, matching SelectGroup/SelectRole/SelectRequest.
The sort is stable, so colliding rows keep their input order. Guard order is
unchanged: non-interactive still fires before the empty-list check.

SelectSessions keeps its display-text lookup and gains a comment saying why:
every option string embeds the session ID, so collisions are unreachable.

Tests:
- TestResolveTargetSelection_DuplicateDisplayStrings pins the index path; it
  fails when the resolver is reverted to a display lookup.
- TestResolveTargetSelection_OutOfRange mirrors the group equivalent.
- TestSortTargetsForDisplay_Ordering pins display ordering, stable ordering of
  colliding rows, and caller-slice immutability.

Docs: CHANGELOG Fixed entry; mutation-ledger production-changes row (not an
audit row - found by the PR7 review), UI-06 line reference repointed, and both
FindTargetByDisplay and FindGroupByDisplay noted as deletion candidates.
The unified selector behind plain `grant`, `grant --groups` and
`grant favorites add` still bound survey.Select to a string and looked the
answer back up with findItemByDisplay. formatSelectionItem carries no ID, so
two Entra ID groups with the same name in different directories render
identically and the lookup returned the first match regardless of which row the
user highlighted — elevating into the wrong group.

Bind the selected index instead and recover the item with a bounds-checked
resolveSelectionItem, mirroring resolveGroupSelection/resolveTargetSelection.
Out-of-range indexes error rather than clamp. buildUnifiedOptions now sorts
stably so the options slice and the sorted items slice stay index-aligned.
The index fixes were only pinned by unit tests over the resolve* helpers, so
reverting SelectTarget/SelectGroup/SelectItem to display-string resolution left
the whole suite green. These tests drive the real survey prompt over a real
pseudo-terminal and assert the highlighted row is the one that comes back,
including after the user types a filter.

The pty is allocated with raw /dev/ptmx plus the TIOCSPTLCK/TIOCGPTN ioctls so
no new Go module dependency is needed. Linux-only by file name; CI's Windows
leg never compiles these files.
ui.SelectGroup, ui.FindGroupByDisplay and ui.FindTargetByDisplay had zero
production callers: the group path users actually reach (`grant --groups` and
plain `grant`) goes through cmd's unified selector, and both Find* helpers were
superseded by index resolution. Removing SelectGroup orphans
sortGroupsForDisplay and resolveGroupSelection, which existed only to serve it,
so they go too, along with the tests that only covered the deleted code.

ui.SelectTarget stays — it is live via `grant env`. FormatGroupOption and
BuildGroupOptions stay — cmd/list.go uses them.
Both sorted selectors were only exercised with 4-element fixtures. Go's pdqsort
delegates to insertion sort below n=12, which is incidentally stable, so
swapping sort.SliceStable for sort.Slice passed the whole suite. The new
fixtures use 15 entries in 5 colliding groups and assert that items rendering
identically keep their input order, which kills that mutation in both places.
The ledger credited the two index fixes to tests over the extracted resolve*
helpers, which do not pin the wiring — reverting the selectors to
display-string resolution left the suite green. Name the pty tests instead, and
add rows for the unified-selector fix, the dead-code deletion and the widened
sort fixtures.

A test comment claimed survey.Select cannot be driven from a test; it can, and
now is. The SelectSessions comment asserted collisions are impossible; that is
an assumption about the SCA API, so state it as one and note that
FindSessionByDisplay fails open if it breaks.

CHANGELOG: collapse the two selector lines into one that covers both the
unified selector and grant env's target selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant